Skip to content

Conversation

@joelddiaz
Copy link

@joelddiaz
Copy link
Author

/hold wait on openshift/cloud-credential-operator#433

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 17, 2021
@patrickdillon
Copy link
Contributor

/test e2e-azure

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable to me. #5325 is a start to clean up some loose ends in how the installer handles credential modes (I think we want to move toward choosing defaults rather than allowing credentialmode to be ""). #5325 would need to handle this case as well, where we have two modes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue with choosing a default instead of an empty string is that they mean different things. When an explicit selection is made, it (1) tells the cloud-credential-operator to use that mode and (2) skips permissions validation. When an empty string is used, it tells the cloud-credential-operator to inspect the credentials provided and make an opinionated decision about which mode to use based upon what is allowed with the credentials.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, now with Azure there is only Passthrough. So no more intelligent querying of the creds.

@staebler
Copy link
Contributor

staebler commented Feb 1, 2022

/hold wait on openshift/cloud-credential-operator#433

@joelddiaz Are we ready to remove the hold on this PR?

@staebler
Copy link
Contributor

staebler commented Feb 1, 2022

/lgtm
/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 1, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: staebler

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2022
@staebler
Copy link
Contributor

staebler commented Feb 1, 2022

Changes need to be made to

// Azure: "Mint", "Passthrough", "Manual"
as well.

/lgtm cancel

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 1, 2022
@joelddiaz
Copy link
Author

/unhold

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 1, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 1, 2022

New changes are detected. LGTM label has been removed.

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 1, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 1, 2022

@joelddiaz: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-alibaba 65831b75cb3dc8f2b9f597f4ccc6d3f61fc23ceb link true /test e2e-alibaba
ci/prow/e2e-crc 0d828c2 link false /test e2e-crc
ci/prow/e2e-aws-upgrade 0d828c2 link true /test e2e-aws-upgrade
ci/prow/okd-e2e-aws-upgrade 0d828c2 link false /test okd-e2e-aws-upgrade
ci/prow/unit 0d828c2 link true /test unit
ci/prow/e2e-aws-single-node 0d828c2 link false /test e2e-aws-single-node
ci/prow/e2e-aws-workers-rhel7 0d828c2 link false /test e2e-aws-workers-rhel7
ci/prow/e2e-aws-workers-rhel8 0d828c2 link false /test e2e-aws-workers-rhel8
ci/prow/e2e-azure-upi 0d828c2 link false /test e2e-azure-upi
ci/prow/e2e-ibmcloud 0d828c2 link false /test e2e-ibmcloud
ci/prow/okd-unit 0d828c2 link true /test okd-unit
ci/prow/e2e-openstack-kuryr 0d828c2 link false /test e2e-openstack-kuryr
ci/prow/e2e-aws-fips 0d828c2 link false /test e2e-aws-fips

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@patrickdillon
Copy link
Contributor

/close
in favor of #5699

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 17, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 17, 2022

@joelddiaz: PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 17, 2022

@patrickdillon: Closed this PR.

Details

In response to this:

/close
in favor of #5699

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci openshift-ci bot closed this Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants